Add programmatic search endpoint for logs/traces - #2258

🦋 Changeset detected

Latest commit: d92f5af

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vinzee is attempting to deploy a commit to the HyperDX Team on Vercel.

A member of the Team first needs to authorize it.

PR Review

Solid implementation: well-scoped, well-tested (incl. cross-team isolation, semicolon/subquery rejection, pagination determinism), and the error mapping (ClickHouse user-input errors → 400, others → 500) is sensible.

A few non-critical items worth tightening before merge:

• ⚠️ PR description examples don't match the schema → The first curl shows "select": [{ "valueExpression": "Timestamp" }, …] (array of objects), but searchRequestSchema declares select as a comma-separated string. The second example uses "columns": "…", but no columns field exists — it's select. Update the PR body so users don't copy a payload that returns 400.
• ⚠️ OpenAPI SearchRow description references a non-existent columns field (packages/api/src/routers/external-api/v2/search.ts:121-126 — "…the source default columns when columns was omitted") → Change columns → select to match the schema.
• ⚠️ Inconsistent SQL-injection-style hardening across user-controlled SQL fields → select is gated by DISALLOWED_COLUMNS_PATTERN (no ;, no SELECT\s), but orderBy and where (when whereLanguage: "sql") are passed straight through with only a length cap. Either drop the half-measure on select (the real boundary is the read-only CH user), or apply the same regex check to orderBy and SQL-mode where so the surface is consistent.
• ℹ️ PR description claims runConfigTile was extended with an offset option, but the diff only adds a startDate > endDate check to parseTimeRange — the new endpoint uses its own runSearchConfig, not runConfigTile. Worth correcting the description to avoid confusing future readers/auditors.
• ℹ️ Tests only cover Log sources. A quick Trace-source happy path (and/or an explicit reject on Metric/Session sources, if those aren't supported) would round out the integration tests for the "logs/traces" framing in the docs.

No blocking bugs or security issues found.

Deep Review

🔴 P0/P1 -- must fix

• packages/api/src/routers/external-api/v2/utils/search.ts:145 -- orderBy is concatenated verbatim into the rendered ORDER BY via UNSAFE_RAW_SQL, so an authenticated caller can submit orderBy: "(SELECT password_double_sha1_hex FROM system.users LIMIT 1)" or orderBy: "sleepEachRow(3)" and run arbitrary scalar subqueries / expensive functions against every table the source's CH user can read; the Zod schema only enforces max(1024) with no refinement.
  • Fix: Restrict orderBy to an allowlist of <column-ref>(\s+(ASC|DESC))? entries parsed by a real SQL tokenizer, or force readonly=2 plus a per-source database allowlist on the CH query.
  • _{security, adversarial, reliability}
• packages/api/src/routers/external-api/v2/search.ts:201 -- DISALLOWED_COLUMNS_PATTERN = /;|(?<!\w)SELECT\s/i requires literal whitespace after SELECT, so the payload select: "(SELECT/**/groupArray(name) FROM system.users)" passes validation and reaches renderSelectList, which inlines it via UNSAFE_RAW_SQL for the same arbitrary-table-read capability.
  • Fix: Tighten to /;|--|\/\*|(?<!\w)SELECT(?!\w)/i and add a test for the comment-bypass form, or parse the column list with a SQL tokenizer and reject any nested SELECT node.
  • _{security, adversarial, correctness}
• packages/api/src/routers/external-api/v2/utils/search.ts:139 -- when whereLanguage: "sql" the user string is wrapped in UNSAFE_RAW_SQL with no validation, so where: "1 OR (SELECT password FROM system.users) IS NOT NULL" becomes a working error/blind SQL injection oracle through the CH_USER_INPUT_ERRORS -> 400 echo path.
  • Fix: Force readonly=2 and a database allowlist on every CH query from this endpoint, and gate whereLanguage='sql' behind an explicit per-API-key opt-in.
  • _security
• packages/api/src/routers/external-api/v2/utils/search.ts:120-134 -- when the source has no max_execution_time querySetting requestTimeout is undefined, so the ClickhouseClient inherits the @clickhouse/client 1-hour default and there is no AbortSignal wired to req.on('close'), letting a single bad query (or any of the injection payloads above) park an Express worker and a CH connection for up to 60 minutes after the caller disconnects.
  • Fix: Apply a hard 30-60s fallback requestTimeout, wire an AbortController from req.on('close') through runSearchConfig into queryChartConfig({ opts: { abort_signal } }), and set server-side max_execution_time/max_rows_to_read in the per-query SETTINGS.
  • _{reliability, adversarial}

🟡 P2 -- recommended

• packages/api/src/routers/external-api/v2/search.ts:415-428 -- the catch block forwards any thrown error with a numeric statusCode in [400, 499] and echoes its raw err.message, so a dep that throws { statusCode: 400, message: <internal detail> } leaks that detail to the API consumer.
  • Fix: Only honor statusCode for an allowlist of known internal classes (e.g. BaseError, ZodError); otherwise return a sanitized 500.
  • _reliability
• packages/api/src/routers/external-api/v2/utils/search.ts:23-78 -- the comment claims resolveSearchOrderBy mirrors optimizeDefaultOrderBy from DBSearchPage.tsx, but it drops both the tableMetadata.sorting_key fold-in and the implicitDateTimePrefixes allowlist, so a source whose UI emits (TenantId, Timestamp) DESC will here emit Timestamp DESC and lose primary-key-based streaming on wide windows.
  • Fix: Either rename to resolveSearchOrderByFallback and rewrite the comment to drop the parity claim, or hoist the helper into @hyperdx/common-utils and share it with the UI hook.
  • _{correctness, maintainability}
• packages/api/src/routers/external-api/v2/index.ts:46 -- the new route uses the shared defaultRateLimiter (100 req/min/key); combined with maxResults: 2000 + offset: 10000 and an unbounded response buffer this lets a single key sustain ~1.2M CH rows scanned/min and ~2 GB worst-case response heap.
  • Fix: Apply a tighter per-endpoint rate limit, stream the response with a hard byte cap, and add server-side max_rows_to_read / max_bytes_to_read settings.
  • _{security, adversarial}
• packages/api/src/routers/external-api/v2/search.ts:407 -- error responses for SYNTAX_ERROR / UNKNOWN_IDENTIFIER / THERE_IS_NO_COLUMN echo the first 300 chars of the CH message, which typically names the offending column and CH's suggestions, giving the SQL-injection paths above a free schema-probing oracle.
  • Fix: Return a generic "Query syntax error" for the UNKNOWN_* codes and log the detailed chType/safeMsg server-side only.
  • _security
• packages/api/src/routers/external-api/v2/search.ts:397-425 -- both logger.error sites in the catch block omit teamId, sourceId, and the requested dateRange, so per-tenant or per-source error spikes cannot be bucketed from logs alone.
  • Fix: Include { teamId, sourceId, dateRange, chType, safeMsg } in both logger calls.
  • _reliability
• packages/api/src/mcp/tools/query/helpers.ts:37-39 -- the new startDate > endDate rejection runs for every existing caller of parseTimeRange, changing hyperdx_query and hyperdx_query_tile from "successful empty result" to "error" on inverted ranges, but the changeset only mentions the new HTTP endpoint.
  • Fix: Either document the MCP behavior change in the changeset, or move the inverted-range check into the /api/v2/search route and leave the shared helper untouched.
  • _correctness
• packages/api/src/routers/external-api/v2/search.ts:369 -- this endpoint returns errors as { message: string }, but peer /api/v2/charts/series returns { error: string }; the v2 surface no longer has one error envelope clients can rely on.
  • Fix: Align the older endpoints on { message } (matches dashboards) or document the inconsistency.
  • _api-contract
• packages/api/src/routers/external-api/__tests__/search.test.ts -- no test exercises the ClickHouseQueryError -> 400 mapping (the entire CH_USER_INPUT_ERRORS branch), an explicit orderBy request value, a SourceKind.Trace source, the querySettings.max_execution_time -> requestTimeout calculation, or any of resolveSearchOrderBy's explicit/multi-part/displayedTimestamp branches.
  • Fix: Add focused tests for the CH error mapping (e.g. malformed SQL where), an explicit orderBy override, a trace source, a source with max_execution_time, and a source with multi-part timestampValueExpression.
  • _{testing, correctness, maintainability}

Reviewers (9): correctness, security, api-contract, reliability, adversarial, kieran-typescript, testing, maintainability, project-standards.

Testing gaps:

• No test sends a select containing SELECT/**/, an orderBy containing a subquery / sleepEachRow, or whereLanguage='sql' with a scalar subquery — the security boundary for all three fields is unverified.
• No multi-tenant integration test confirms team A cannot read team B via crafted select / where / orderBy when the two teams share a CH connection user.
• No test simulates a slow / disconnected ClickHouse to verify requestTimeout actually fires or that a client disconnect cancels the in-flight query.

@pulpdrew addressed all comments